CI https://ci.nodejs.org/job/node-test-pull-request/13971/

Thanks for doing this! 🙂 Will make interop much cleaner

CI https://ci.nodejs.org/job/node-test-pull-request/13995/

@SimenB I am not exactly sure in what way it will actually improve the interop. The messages could change due to being further improved and I strongly advice against relying on the error message by simply parsing it. If you want to verify input with a assert error message there is actually a good way to do that as described here: https://github.com/nodejs/node/pull/19724/files

It'll improve it in that it allows us to not have to strip away redundant (as we have our own way of conveying the same thing) information. Some combination of strip-ansi and substring on the message could probably achieve the same thing, but this should be cleaner.

We have a way of separating the message from the stack if it's in both places which maybe will be wonky after this, but that should be easier to deal with thanks to the code property already present

I am still not sure I can follow. Let's say we have the following:

assert.strict.deepStrictEqual([1,1,1,1,1,2,2,2,2], [1,1,1,1,1,2,2,2])
// AssertionError [ERR_ASSERTION]: Input A expected to deepStrictEqual input B:
// + expected - actual ... Lines skipped

//   [
//     1,
// ...
//     1,
// -   2,
//     2,
// ...
//     2
//   ]

This would include colors. If the default util.inspect settings are set to contain colors, the numbers will all also be colored. But what can you do with this message? Don't you generate a complete new message in Jest? And what would happen if the message format is slightly changed again?

@BridgeAR Side question: This is labeled author ready and the “docs included” checkbox is checked … Are we not documenting this because we don’t document other properties of these errors too, or am I missing something?

I don’t think it’s a terrible thing if it doesn’t happen in this PR, but as I understand it these properties are intended for public consumption, so we probably should document them eventually, right?

@addaleax in general: absolutely. So far we did not document any of those properties. So I went ahead and opened a PR for that: #19724. As soon as either of them lands, the other one needs a update.

Don't you generate a complete new message in Jest?

We keep the original message (but tweak the stack) in addition to a custom message. See https://github.com/facebook/jest/blob/1eb02fd41ad6840350a5b2c0636151e2da8bcd8b/integration-tests/__tests__/__snapshots__/failures.test.js.snap#L124-L512 for example of all of assert's functions within Jest

I added the documentation after a rebase, since I just landed #19724.

I am still not convinced about adding this functionality. @SimenB I am going to look at the implementation in Jest as I believe there is probably a better way to directly handle this there.

Marking as blocked because of that.

New CI https://ci.nodejs.org/job/node-test-pull-request/14042/

@SimenB I am going to look at the implementation in Jest as I believe there is probably a better way to directly handle this there.

Implementation lives here: https://github.com/facebook/jest/blob/1eb02fd41ad6840350a5b2c0636151e2da8bcd8b/packages/jest-jasmine2/src/assert_support.js

I'm not convinced this change adds real value either.

1. util.inspect.defaultOptions.colors = false
2. Cleaning ANSI codes is simple

So I'll -1

@refack please be aware that it is actually not a good idea to change the util.inspect.defaultOptions. I try to fix that at some point but that turned out to be more work than I originally anticipated. The reason that this is bad, is that it changes the behavior of all code. So a module could actually change that and the user is surprised with the output.

So a module could actually change that and the user is surprised with the output.

Agreed, but still a valid solution for root applications i.e. tools like jest

Ping @refack

IMHO rawMessage should be the only message. That is message shouldn't include anything but text.

So

Should be:

return util.inspect( 
     val, 
     { compact: false, customInspect: false, colors: false } 
   ).split('\n');

How about going with @refack’s suggestion, and then, for color support, add a custom inspect override for assertion errors that could provide the right colorization, if that subsequent inspect call requests it?

@addaleax I am not sure I understand your implementation suggestion correct. Can you please elaborate?

Besides that: I personally believe using colors is a good idea and we should in general try to use more colors in core. It improves the readability a lot out of my perspective. Right now the error message will only contain colors if the TTY supports colors and colors did not get deactivated by the user or the user (or a module) explicitly changed the util.inspect.defaultOptions to always use colors.

@BridgeAR The idea is:

• No color output in .message
• Still, generate a colorized version of the message, and store it on the error object in some way
• Add a [util.inspect.custom](depth, ctx) override on the assertion error which that decides based on ctx.colors

That way we don’t have to deal with color support checking while creating the error, where we can’t yet know how it’s going to end up being used.

FWIW, the thing I want is no colors and no diff, and colors are easy to strip out (https://github.com/chalk/strip-ansi). But the diff forces us to manually split up .message to remove it, so just removing colors from .message doesn't really solve Jest's use case at all.

@SimenB

FWIW, the thing I want is no colors and no diff, and colors are easy to strip out (https://github.com/chalk/strip-ansi). But the diff forces us to manually split up .message to remove it, so just removing colors from .message doesn't really solve Jest's use case at all

That sounds like you want to know about the actual and expected properties from the AssertionError. There are more properties that are currently documented on master that will probably help you out.

@addaleax thanks for explaining your idea in more detail. I am going to give this another thought but right now I still think it would be nicer to add colors as a default even if colors is not explicitly activated.

We want actual and expected yes (and it's great that they're there), but we also want the message provided instead of providing our own. Especially since assert supports a third message argument (e.g. assert.equal('hello', 'world', 'string should match')).

So the rawMessage property suggested here (which allows us to keep using assert the way we currently use it) is perfect.

@addaleax thanks for explaining your idea in more detail. I am going to give this another thought but right now I still think it would be nicer to add colors as a default even if colors is not explicitly activated.

Can you explain why you think that?

@BridgeAR I played around with my suggestion but noticed that we don’t actually use util.inspect() for printing errors in most situations :/

I still think it would be nicer if we didn’t add color support based on feature detection for stdout if we don’t know whether it’ll end up there, but I think given the current constraints your implementation makes sense, so feel free to ignore my comments above :)

@addaleax

Can you explain why you think that?

Most users rely on default values and those are often very old and not optimal. Some times that is for historical reasons but most often just because originally there was no better solution. Now that there is a rough color detection, we should start relying on that so we can provide colors when we determine that the user is capable of handling those. The user can still deactivate those in general.

@refack please take another look. I would like to go ahead and land this if you do not feel strongly against it.

@refack PTAL

The changed error message (diff and the wording around the operator) is the only thing breaking Jest's test suite using 10-rc.1. It'd be great to land a solution for this one way or the other. The changed wording we can work around in our test, but the double diff looks weird, so that'll require a code change in Jest. rawMessage would be perfect, but we can probably slice up the error message manually as well if needed.

To be clear, for consumers of Jest this is not really an issue (double diff won't break anybody, it just takes up more space in the terminal - although we'll want to remove it in the long run), it's just Jest's own assertions that the interop works which break.

(This message looka kinda passive-aggresive even to my own eyes. That's not my intention at all ❤️).

EDIT: Most of the discussion in this PR has been about the colors - those aren't really an issue for us (we can use strip-ansi). It's the diff that I want to avoid 🙂

@SimenB I am not sure what you mean with "double diff". I guess you mean Jest creates a diff and Node.js creates a diff as well? The rawMessage as it is in here would contain a diff as well and would not change anything about that. It would only make sure there are no color codes in there.

I still recommend to just rely on the properties of the thrown error instead of relying on the message. That seems to be best for Jest. To be more independent in tests see e.g., https://github.com/eslint/eslint/pull/10182/files.

@SimenB I am not sure what you mean with "double diff". I guess you mean Jest creates a diff and Node.js creates a diff as well?

Correct. See this screengrab using rc.1:

The flipped around + and - for instance is not really acceptable - it's super-confusing for consumers (not to start a discussion about which notation is most correct).

The rawMessage as it is in here would contain a diff as well and would not change anything about that.

Oh, I though rawMessage would basically be the message in node <=9 (I haven't looked at the actual diff here). If it's just colors then it doesn't really do anything for Jest's use case at all.

I still recommend to just rely on the properties of the thrown error instead of relying on the message.

While setting up some code examples to show what I mean, I found the property generatedMessage which is false when message is provided by the user. That means we can actually just remove message if generatedMessage === true. That should solve our problem! 🎉

generatedMessage is not documented here: https://nodejs.org/dist/latest-v8.x/docs/api/assert.html, it probably should be.

Let me make a quick test to see if we can just move to that, and I'll report back.

EDIT: Yeah, that's enough for Jest. Just doing a e.stack.replace(e.message, '') removes the diff from the stack as well, at which point we're good!

So, at least for Jest's use case, this is not needed 🙂

@SimenB

The flipped around + and - for instance is not really acceptable

That is a very valid point. I am going to check what is most commonly used pattern and improve that if it there is a "standard" to it that is actually flipped to the current implementation.

generatedMessage is not documented here

The docs landed recently as I pointed out above (#19723 (comment), #19723 (comment) & #19723 (comment)).

That means we can actually just remove message if generatedMessage === true. That should solve our problem!

🎉 🎈

If it's just colors then it doesn't really do anything for Jest's use case at all.

Yes, I guess then there is no real reason to actually land this. I am going to close the PR therefore.

Something in general: Thanks a lot for the constructive feedback. There were some great findings and I am going to try to keep an closer eye on Jest.

Thank you for the patience!

For posterity, PR in Jest for node 10 support: jestjs/jest#6055

EDIT: Also working on getting Jest into CITGM, should highlight potential issues at least 🙂 nodejs/citgm#560